Correct categorization analyzer in ES|QL categorize #117695
Conversation
jan-elastic
added
>non-issue
:ml
|
Pinging @elastic/ml-core (Team:ML) |
.../src/test/java/org/elasticsearch/compute/aggregation/blockhash/CategorizeBlockHashTests.java
Show resolved
Hide resolved
| COUNT():long | category:keyword | ||
| 1 | .*?\.\*\?Connected\.\+\?to\.\*\?.*? | ||
| 1 | .*?\.\*\?Connection\.\+\?error\.\*\?.*? | ||
| 1 | .*?\.\*\?Disconnected\.\*\?.*? |
There was a problem hiding this comment.
The categorization analyzer is more aggressive w.r.t. specials chars, hence these changes.
| ; | ||
|
|
||
| COUNT():long | category:keyword | ||
| 2 | .*?DB.+?servers.*? |
There was a problem hiding this comment.
DB is parsed as a hex number and dropped. Obviously suboptimal, but unrelated to this PR.
...te/src/main/java/org/elasticsearch/compute/aggregation/blockhash/CategorizeRawBlockHash.java
Show resolved
Hide resolved
jan-elastic
force-pushed
the
esql-categorize-analyzer
branch
2 times, most recently
from
ef76db0 to
7f61b4e
Compare
There was a problem hiding this comment.
Thanks @jan-elastic ! I have mostly minor comments; the only comment I care a bit more about is whether we need to pass the analysis registry as far down as we currently do, but even so this could be merged as-is.
...te/src/main/java/org/elasticsearch/compute/aggregation/blockhash/CategorizeRawBlockHash.java
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/compute/aggregation/blockhash/CategorizeBlockHashTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
nit: To show that the analysis registry is used correctly, could we maybe add one more test that uses a different analysis registry that leads to a different categorization?
There was a problem hiding this comment.
I'm not sure how to write such test. How do I construct a different analysis registry, that contains different versions of the necessary filters/tokenizers. Sounds like quite some effort with low reward.
| 1 | .*?😊👍🏽.+?detcennocsiD.*? | ||
| 3 | .*?😊👍🏽.+?ot.+?detcennoC.*? | ||
| 3 | .*?😊👍🏽.+?rorre.+?noitcennoC.*? | ||
| 1 | .*?detcennocsiD.*? | ||
| 3 | .*?ot.+?detcennoC.*? | ||
| 3 | .*?rorre.+?noitcennoC.*? |
There was a problem hiding this comment.
@ivancea , this change will solve the recent failing tests, no?
There was a problem hiding this comment.
Yes, pretty sure it does
| enrichLookupService, | ||
| lookupFromIndexService, | ||
| new EsPhysicalOperationProviders(contexts) | ||
| new EsPhysicalOperationProviders(contexts, searchService.getIndicesService().getAnalysis()) |
There was a problem hiding this comment.
Maybe we could actually already build the categorizer here, to avoid unnecessary dependencies on the analysis registry?
There was a problem hiding this comment.
As said in a different comment: in the future the analyzer should be configurable, and to achieve I think that it should be wired like this. (I agree that for now we could have created the fixed analyzer here)
There was a problem hiding this comment.
Not so sure anymore, see #117695 (comment)
jan-elastic
force-pushed
the
esql-categorize-analyzer
branch
from
7f61b4e to
8ebf21e
Compare
* Correct categorization analyzer in ES|QL categorize * close categorizer if constructing analyzer fails * Rename capability CATEGORIZE_V4 * add comments
No description provided.